-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(delta/sds): Mark secret as ready if server says the resource doesn't exist #33362
(delta/sds): Mark secret as ready if server says the resource doesn't exist #33362
Conversation
… exist Signed-off-by: Keith Mattix II <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nits but otherwise LGTM, thanks!
Assigning Harvey for senior-maintainer review.
/assign @htuch
Does this need runtime protection? Seems pretty minor corner case, so I'm inclined to say no, but just putting it out there. |
Runtime protection in what sense? |
Signed-off-by: Keith Mattix II <[email protected]>
As per https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md#breaking-change-policy. I think it's fine TBH in this case, but for next behavior change it's worth familiarising. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the version history to be clear this changed?
IIUC, when you take both of my PRs together, the only change to the user is that an SDS removal of a secret that doesn't exist yet is NACK'd instead of ACK'd. Based on the breaking change policy (thanks for the link!), the first PR was the breaking change but this PR reverts to the status quo with respect to the init target being marked as ready. Sorry if I'm misunderstanding something |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, makes sense, thanks.
… exist (envoyproxy#33362) As discussed in Slack, a delta removal should complete cluster warming Risk Level: Low Testing: integration Signed-off-by: Keith Mattix II <[email protected]>
Istio has very few maintainers here and though I'm still learning, I've contributed a handful of upstream envoy PRs as well as 2 recent changes to istio-proxy's telemetry. If existing maintainers have other areas they'd like to see more contributions in, I welcome the feedback! PRs: - envoyproxy/envoy#35074 - envoyproxy/envoy#33857 - envoyproxy/envoy#33362 - envoyproxy/envoy#32961 - istio/proxy#5617 - istio/proxy#5514
Istio has very few maintainers here and though I'm still learning, I've contributed a handful of upstream envoy PRs as well as 2 recent changes to istio-proxy's telemetry. If existing maintainers have other areas they'd like to see more contributions in, I welcome the feedback! PRs: - envoyproxy/envoy#35074 - envoyproxy/envoy#33857 - envoyproxy/envoy#33362 - envoyproxy/envoy#32961 - istio/proxy#5617 - istio/proxy#5514
As discussed in Slack, a delta removal should complete cluster warming
Commit Message: (fix/sds): Mark cluster as ready after
Additional Description:
Risk Level: Low
Testing: integration
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]